png: Make bytemuck optional #138
Conversation
|
Hi, thanks for your contribution. I don't see any problem with the code, but pinging @Shnatsel in case there is some memory bug introduced here. I'm not a fan of nested functions so I'd rather the function be moved to be a standalone (probably in utils.h) But in case there are no bugs, we should probably drop the bytemuck dependency all together |
|
Looks good to me, incl. dropping bytemuck entirely. I'd like to see a more detailed safety comment. Calling out that alignment of u8 always less than u16, and both types are valid for all bit patterns with no lifetimes so a transmute is fine ( Also we should leave a comment that if more such conversions are needed, |
zune-png/src/decoder.rs
Outdated
| @@ -777,7 +777,22 @@ impl<T: ZReaderTrait> PngDecoder<T> { | |||
| { | |||
| &mut out_u8 | |||
| } else { | |||
| let (a, b, c) = bytemuck::pod_align_to_mut::<u16, u8>(&mut out_u16); | |||
| #[inline(always)] | |||
There was a problem hiding this comment.
This can be greatly simplified by just dropping all of this alignment logic. Converting a 16 slice to a u8 slice is always correct because the alignment of the target is smaller. Just casting it by hand and doubling the size is easier to understand than the align_to_mut dance imo.
There was a problem hiding this comment.
The bytemuck https://docs.rs/bytemuck/latest/bytemuck/fn.bytes_of_mut.html function could be used but if bytemuck is optional it probably makes more sense to just drop it
I personally prefer transmute to align_to, because align_to behavior is not specified But this change brakes the promise of a879297 (attached the commit for reference, would rather remove it later on rebase or something) |
zune-png/src/utils.rs
Outdated
| pub fn convert_u16_to_u8_slice(slice: &mut [u16]) -> &mut [u8] { | ||
| // Converting a u16 slice to a u8 slice is always correct because | ||
| // the alignment of the target is smaller. | ||
| unsafe { std::slice::from_raw_parts_mut(slice.as_ptr() as *mut u8, slice.len() * 2) } |
There was a problem hiding this comment.
What if slice.len() * 2 overflows?
There was a problem hiding this comment.
I don't think it can but there should be a comment explaining why it cannot
There was a problem hiding this comment.
I also thought it can not, but fixed it with a checked_mul.
not-fl3
force-pushed
the
dev
branch
2 times, most recently
from
96f430c
to
336d36d
Compare
Fine by me, it's one well documented line |
For this, btw I'd love if it went to |
|
@not-fl3 Is this okay to merge? |
not-fl3
force-pushed
the
dev
branch
3 times, most recently
from
2f0ae47
to
6012595
Compare
Converting a 16 slice to a u8 slice is always correct because the alignment of the target is smaller.
I believe so! Rebased out my first try commit. |
|
Thank you, will make a breaking release probably over the weekend with the changes |
For
zune-png,bytemuck, the crate, is used just once, to convert u16 slice to u8 slice.This PR does not alter the default behavior: this conversion is still going through the crate.
But without
bytemuckfeature it is justslice.align_to_mut::<u8>().I tested the new code on the hdr PNG triggering this codepath and it all seems to still work fine.
New function looks ugly, but, unfortunately, attributes on regions are not yet stable and that was the simplest workaround I've seen.
I do have a PR for the
login queue :D Not likebytemuckandlogare that big, but having them under a feature-flag would be really nice!